Add python opener from rasterio 1.4 #1331
Conversation
* Modern _path module and updated usage * A setup.cfg for editable installs * A Dockerfile and Makefile for testing Resolves #1328
sgillies
changed the title
| @@ -0,0 +1,30 @@ | |||
| ARG GDAL=ubuntu-small-3.6.4 | |||
| FROM gdal | ||
| COPY . . | ||
| RUN /venv/bin/python -m build -o wheels | ||
| RUN /venv/bin/python -m pip install --no-index -f wheels fiona[test] |
There was a problem hiding this comment.
It's been good to have a chance to revisit the Dockerfile. This is the way to build and install packages now.
sgillies
changed the title
|
The test failures are with Python 3.7 and 3.8, which are being left behind by Numpy. Fiona 1.10 will drop support for them, too. |
| @@ -41,6 +35,7 @@ dependencies = [ | |||
| "click-plugins>=1.0", | |||
| "cligj>=0.5", | |||
| 'importlib-metadata;python_version<"3.10"', | |||
| "numpy>=1.25,<2", | |||
There was a problem hiding this comment.
It's not needed to require a numpy 1.25 as a minimum version. The goal of building with a minimum version of 1.25 is that this ensures it also runs with older numpys
(in contrast to older versions of numpy, where building with that specific version meant that the wheels were only compatible with that version or up, hence the use of "oldest-supported-numpy" for wheel building)
See https://numpy.org/doc/stable/release/1.25.0-notes.html#compiling-against-the-numpy-c-api-is-now-backwards-compatible-by-default in the 1.25 release notes, and https://numpy.org/doc/stable/dev/depending_on_numpy.html#build-time-dependency
| "oldest-supported-numpy", | ||
| "setuptools>=67.8", | ||
| "wheel", | ||
| ] | ||
| requires = ["setuptools>=67.8", "cython~=3.0.2", "numpy>=1.25,<2"] |
There was a problem hiding this comment.
Another question, seeing numpy included here: do you know for what reason fiona actually did have a build requirement on numpy before? A search in the codebase doesn't actually show any use of numpy (before this PR)
It seems this was brought in with 37da85d, which copied some code from rasterio (and rasterio indeed depends on numpy, but AFAIK Fiona does not?)
| cdef size_t pyopener_write(void *pFile, void *pBuffer, size_t nSize, size_t nCount) with gil: | ||
| cdef object file_obj = <object>pFile | ||
| buffer_len = nSize * nCount | ||
| cdef np.uint8_t [:] buff_view = <np.uint8_t[:buffer_len]>pBuffer |
There was a problem hiding this comment.
As far as I can see, this is the only use of numpy in this PR. I think this could be easily avoided by using a builtin cython type instead of a numpy type. Something like unsigned char instead of np.uint8_t
That would also avoid having to add numpy as a build dependency to start with?
There was a problem hiding this comment.
@jorisvandenbossche yes, I'll definitely look into removing the hard numpy requirement before the next pre-release.
| @@ -60,7 +62,7 @@ | |||
| _remove, | |||
| _remove_layer, | |||
| ) | |||
| from fiona.path import ParsedPath, parse_path, vsi_path | |||
| from fiona._path import _ParsedPath, _UnparsedPath, _parse_path, _vsi_path | |||
There was a problem hiding this comment.
FWIW, I see you added a deprecation warning inside the fiona.path module. But the above change directly breaks code that was using those functions from top-level fiona instead of from fiona.path
We are running into some issues with this in geopandas: geopandas/geopandas#3207
There was a problem hiding this comment.
I commented at geopandas/geopandas#3207. Fiona's path module and methods are only public by accident. Other projects shouldn't be coupled to fiona's internals, which are full of weird details. Removing this coupling between geopandas and fiona shouldn't be hard and I'm happy to help.
And supporting changes:
See also rasterio/rasterio#3032
Resolves #1328